Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mention every Transport Parameter in section on values for 0-RTT #3756

Merged
merged 3 commits into from Jun 23, 2020

Conversation

nharper
Copy link
Contributor

@nharper nharper commented Jun 10, 2020

The section for "Values of Transport Parameters for 0-RTT" specifies
which parameters a client MUST or MUST NOT remember. It does not do the
same for a server. (It implies that ones a client MUST NOT remember also
don't need to be remembered by a server, and it lists a subset of the
remaining as ones the server MUST remember.) This commit adds the
remaining parameters to a list that the server need not remember.

Fixes #3755

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason that this is left unstated is the problem I've identified below. I would prefer not to do this.

@@ -1749,6 +1749,16 @@ values for 0-RTT. This includes initial_max_data and either
initial_max_streams_bidi and initial_max_stream_data_bidi_remote, or
initial_max_streams_uni and initial_max_stream_data_uni.

When accepting 0-RTT data, a server may set values for the following parameters
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
When accepting 0-RTT data, a server may set values for the following parameters
When accepting 0-RTT data, a server MAY set values for the following parameters

?

values it previously sent.

* max_idle_timeout
* max_udp_payload_size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this does affect 0-RTT. Though we accepted @kazuho's argument that this manifests in a fashion indistinguishable from a change in Path MTU, that doesn't mean that changing it has no consequence, as this text implies.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What transport parameters a server sends cannot, as currently specified, affect 0-RTT, because a client MUST only use remembered transport parameters when sending 0-RTT packets. If changing the max_udp_payload_size does somehow affect 0-RTT, then we need to revisit more of the 0-RTT Transport Parameter handling.

As currently specified, there is no prohibition on the server changing (including reducing) the max_udp_payload_size when accepting 0-RTT. As far as I can figure out, the only implication for accepting 0-RTT and reducing max_udp_payload_size is that some 0-RTT packets might get lost. If the server remembers this parameter, it could choose to reject 0-RTT which explicitly signals to the client to retransmit all frames. The server (whether or not it remembers this parameter) could also accept 0-RTT, process whichever packets get through, and let the client's loss/recovery algorithm retransmit frames. It seems like we end up at the same place either way, possibly with different performance characteristics.

What I'm trying to say is "it is not a requirement to remember parameters on this list". I'm not trying to say "don't remember parameters on this list".

Should we add some language here to state that a server may wish to remember some of these?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be a good idea to change the reasoning to something like: "Because the previous values set for these parameters do not affect what can be carried by 0-RTT packets"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like this: "While changing the value of these parameters can degrade performance of this connection, the connection is expected to proceed correctly. It is therefore acceptable for the server to not recover the values it previously sent."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that sounds reasonable. I'll work on wording to say something along those lines and "your implementation is not incorrect if it chooses not to remember those values, but there may be optimizations that could involve remembering them".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether this is an optimization or not depends on the implementations in question. Basically, I think this boils down to what do we want default behavior to be. I would suggest saying that there could be performance penalties for not remembering them, simply because a server that doesn't remember max_udp_payload_size but drops packets that are larger will cause perf penalties.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can someone provide an example of how remembering max_udp_payload_size can lead to an optimization? I’m failing to see how remembering it can provide benefits

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the number is large relative to the PMTU, then there is no benefit. If the client remembers PMTU it might choose to attempt 0-RTT using the same PMTU. That allows for a larger congestion window and more data transferred in 0-RTT. That might be more requests, which is a tiny benefit at best.

However, if the server forgets the max_udp_payload_size, and has a lower value now than the remembered PMTU, those 0-RTT packets will be dropped as they arrive. If the server accepts 0-RTT in that case without considering the effect of max_udp_payload_size, then all those packets will be considered by the client to be lost. The client will tell its congestion controller and reduce the congestion window. Rather than staying in slow start, the client will exit very early and performance will be terrible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks @martinthomson that makes sense! Based on that I agree that mentioning this could help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all for the comments on this. I reworded this paragraph choosing not to mention whether changing these values can affect 0-RTT data, and explicitly mentioned the perf impact. (I also added an RFC 2119 "OPTIONAL".)

@martinthomson martinthomson added -transport editorial An issue that does not affect the design of the protocol; does not require consensus. labels Jun 10, 2020
The section for "Values of Transport Parameters for 0-RTT" specifies
which parameters a client MUST or MUST NOT remember. It does not do the
same for a server. (It implies that ones a client MUST NOT remember also
don't need to be remembered by a server, and it lists a subset of the
remaining as ones the server MUST remember.) This commit adds the
remaining parameters to a list that the server need not remember.
Copy link
Contributor

@DavidSchinazi DavidSchinazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the latest text!

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

Comment on lines 1752 to 1754
It is OPTIONAL for a server to be able to recover the previously sent values of
the max_idle_timout, max_udp_payload_size, and disable_active_migration
parameters. Lowering the values of these parameters while also accepting 0-RTT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that OPTIONAL and RECOMMENDED are in RFC 2119, but they encourage the passive voice, .

Suggested change
It is OPTIONAL for a server to be able to recover the previously sent values of
the max_idle_timout, max_udp_payload_size, and disable_active_migration
parameters. Lowering the values of these parameters while also accepting 0-RTT
A server MAY choose to recover the previously sent values of the
max_idle_timout, max_udp_payload_size, and disable_active_migration parameters
and reject 0-RTT if it selects smaller values.
Lowering the values of these parameters while also accepting 0-RTT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that active voice is better than passive voice. However, I think this suggested change changes the meaning slightly: saying the "server MAY choose to recover" refers to the server making a choice at the time it would be recovering, while "OPTIONAL for a server to be able to recover" refers to the server making a choice at the time it is storing (or otherwise encoding) parameters to be able to recover at a future point in time. I'm trying to be more clear that the option (whether active voice MAY or passive voice OPTIONAL) is when storing the transport parameters, not when recovering.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "MAY choose to recover" to "MAY store and recover". @nharper, any other way of making this active is also good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"MAY choose to store and recover" sounds good to me.

Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modulo the editorial nit, this LGTM.

Comment on lines 1752 to 1754
It is OPTIONAL for a server to be able to recover the previously sent values of
the max_idle_timout, max_udp_payload_size, and disable_active_migration
parameters. Lowering the values of these parameters while also accepting 0-RTT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change "MAY choose to recover" to "MAY store and recover". @nharper, any other way of making this active is also good.

draft-ietf-quic-transport.md Outdated Show resolved Hide resolved
draft-ietf-quic-transport.md Show resolved Hide resolved
@nharper
Copy link
Contributor Author

nharper commented Jun 23, 2020

This still looks good with the latest commit Jana pushed.

@janaiyengar janaiyengar merged commit eddb3de into quicwg:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List all transport parameters in "Values of Transport Parameters for 0-RTT" section
6 participants